Reviewable status: 0 of 2 files reviewed, 1 unresolved discussion, not enough approvals from maintainers (2 more required), not enough approvals from different teams (1 more required, approved so far: Intel) (waiting on @dimakuv)

Documentation/index.rst line 408 at r1 (raw file):

command-line option to :command:`docker run`.

.. envvar:: GSC_GRAMINE_BINARY

We don't have a different Gramine binary for GSC, it'll still be a gramine binary, hence I would suggest keeping GRAMINE_BINARY instead of GSC_GRAMINE_BINARY envvar.

Reviewable status: 0 of 2 files reviewed, all discussions resolved, not enough approvals from maintainers (2 more required), not enough approvals from different teams (1 more required, approved so far: Intel)

Documentation/index.rst line 408 at r1 (raw file):

Done

Reviewable status: 0 of 2 files reviewed, 1 unresolved discussion, not enough approvals from maintainers (1 more required), not enough approvals from different teams (1 more required, approved so far: Intel) (waiting on @dimakuv)

a discussion (no related file):

(1) Gramine users don't need to know the meaning of word "PAL"
(2) the value "Linux" doesn't correspond to known-to-users gramine-direct
(3) it requires special logic in apploader code.

I think I agree with these, but I'm not sure about the new name, the thing you're supposed to pass there is actually not a binary and you aren't supposed to provide a full path. Maybe GRAMINE_BACKEND or GRAMINE_MODE? And then just pass direct or sgx.

Reviewable status: 0 of 2 files reviewed, 1 unresolved discussion, not enough approvals from maintainers (1 more required), not enough approvals from different teams (1 more required, approved so far: Intel) (waiting on @mkow)

a discussion (no related file):

Done, changed to GRAMINE_MODE

Reviewed 2 of 2 files at r3, all commit messages.
Reviewable status: all files reviewed, 2 unresolved discussions, not enough approvals from maintainers (1 more required), not enough approvals from different teams (1 more required, approved so far: Intel) (waiting on @dimakuv)

Documentation/index.rst line 436 at r3 (raw file):

This is deprecated in GSC v1.8

Didn't you remove it completely in this PR?

templates/apploader.common.template line 12 at r3 (raw file):

# Note: default to SGX if Gramine mode (`direct`, `sgx`) wasn't specified
exec gramine-${GRAMINE_MODE:-sgx} /gramine/app_files/entrypoint \

I don't like it, passing wrong value in this env will cause a weird failure (instead of a user-friendly error).

Code quote:

gramine-${GRAMINE_MODE:-sgx}

Reviewable status: all files reviewed, 2 unresolved discussions, not enough approvals from maintainers (1 more required), not enough approvals from different teams (1 more required, approved so far: Intel) (waiting on @mkow)

Documentation/index.rst line 436 at r3 (raw file):

Oops, true :) Should I keep GSC_PAL logic in apploader.common.template? Or it's so infrequently used that I can just remove it completely?

Well, it's in the documentation, so I guess I should keep that logic but add a warning.

templates/apploader.common.template line 12 at r3 (raw file):

The failure message is not that weird:

/gramine/app_files/apploader.sh: line 16: exec: gramine-bla: not found

You still don't like it and would like to have a proper if-then-else check in this bash script?

Reviewable status: all files reviewed, 2 unresolved discussions, not enough approvals from maintainers (1 more required), not enough approvals from different teams (1 more required, approved so far: Intel) (waiting on @dimakuv)

Documentation/index.rst line 436 at r3 (raw file):

There should at least be a warning (but ideally keep it working + a warning for v1.8). Right now it will just run the SGX version and silently ignore user-provided GSC_PAL=Linux.

templates/apploader.common.template line 12 at r3 (raw file):

Yeah, I think I'd prefer a more explicit error pointing to the env variable which was set incorrectly. It's always nicer for the users, this mistake is rather easy to make and it's just one simple if in Bash to improve it.

Reviewable status: 1 of 2 files reviewed, 2 unresolved discussions, not enough approvals from maintainers (1 more required), not enough approvals from different teams (1 more required, approved so far: Intel), "fixup! " found in commit messages' one-liners (waiting on @mkow)

Documentation/index.rst line 436 at r3 (raw file):

Done.

templates/apploader.common.template line 12 at r3 (raw file):

Done.

Reviewed 1 of 1 files at r4, all commit messages.
Reviewable status: all files reviewed, 1 unresolved discussion, "fixup! " found in commit messages' one-liners (waiting on @dimakuv)

templates/apploader.common.template line 22 at r4 (raw file):

if [ -n "$GSC_PAL" ]; then
    echo "WARNING: GSC_PAL environment variable is deprecated in v1.8 and will be removed in v1.9."
    echo "         Use instead the GRAMINE_MODE={direct|sgx} environment variable."

Suggestion:

Instead, use GRAMINE_MODE={direct|sgx}.

Reviewable status: 1 of 2 files reviewed, 1 unresolved discussion, not enough approvals from maintainers (1 more required), not enough approvals from different teams (1 more required, approved so far: Intel) (waiting on @mkow)

templates/apploader.common.template line 22 at r4 (raw file):

if [ -n "$GSC_PAL" ]; then
    echo "WARNING: GSC_PAL environment variable is deprecated in v1.8 and will be removed in v1.9."
    echo "         Use instead the GRAMINE_MODE={direct|sgx} environment variable."

Done.

Reviewed 1 of 1 files at r5, all commit messages.
Reviewable status: complete! all files reviewed, all discussions resolved